❮

Typo in pg_db_role_setting.h

  • Jump to comment-1
    japinli@hotmail.com2022-08-01T09:18:39+00:00
    Hi, hackers I think there is a typo in pg_db_role_setting.h, should we fix it? diff --git a/src/include/catalog/pg_db_role_setting.h b/src/include/catalog/pg_db_role_setting.h index 45d478e9e7..f92e867df4 100644 --- a/src/include/catalog/pg_db_role_setting.h +++ b/src/include/catalog/pg_db_role_setting.h @@ -51,7 +51,7 @@ DECLARE_TOAST_WITH_MACRO(pg_db_role_setting, 2966, 2967, PgDbRoleSettingToastTab DECLARE_UNIQUE_INDEX_PKEY(pg_db_role_setting_databaseid_rol_index, 2965, DbRoleSettingDatidRolidIndexId, on pg_db_role_setting using btree(setdatabase oid_ops, setrole oid_ops)); /* - * prototypes for functions in pg_db_role_setting.h + * prototypes for functions in pg_db_role_setting.c */ extern void AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt); extern void DropSetting(Oid databaseid, Oid roleid); -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
    • Jump to comment-1
      john.naylor@enterprisedb.com2022-08-01T12:46:30+00:00
      On Mon, Aug 1, 2022 at 4:18 PM Japin Li <japinli@hotmail.com> wrote: > > > Hi, hackers > > I think there is a typo in pg_db_role_setting.h, should we fix it? > > diff --git a/src/include/catalog/pg_db_role_setting.h b/src/include/catalog/pg_db_role_setting.h > index 45d478e9e7..f92e867df4 100644 > /* > - * prototypes for functions in pg_db_role_setting.h > + * prototypes for functions in pg_db_role_setting.c > */ You are correct, but I wonder if it'd be better to just drop the comment entirely. I checked a couple other random headers with function declarations and they didn't have such a comment, and it's kind of obvious what they're for. -- John Naylor EDB: http://www.enterprisedb.com
      • Jump to comment-1
        japinli@hotmail.com2022-08-01T14:24:24+00:00
        On Mon, 01 Aug 2022 at 20:46, John Naylor <john.naylor@enterprisedb.com> wrote: > On Mon, Aug 1, 2022 at 4:18 PM Japin Li <japinli@hotmail.com> wrote: >> >> >> Hi, hackers >> >> I think there is a typo in pg_db_role_setting.h, should we fix it? >> >> diff --git a/src/include/catalog/pg_db_role_setting.h > b/src/include/catalog/pg_db_role_setting.h >> index 45d478e9e7..f92e867df4 100644 >> /* >> - * prototypes for functions in pg_db_role_setting.h >> + * prototypes for functions in pg_db_role_setting.c >> */ > > You are correct, but I wonder if it'd be better to just drop the comment > entirely. I checked a couple other random headers with function > declarations and they didn't have such a comment, and it's kind of obvious > what they're for. Both are fine for me. I find there are some headers also have such a comment, like pg_enum, pg_range and pg_namespace. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
      • Jump to comment-1
        tgl@sss.pgh.pa.us2022-08-01T14:16:08+00:00
        John Naylor <john.naylor@enterprisedb.com> writes: > You are correct, but I wonder if it'd be better to just drop the comment > entirely. I checked a couple other random headers with function > declarations and they didn't have such a comment, and it's kind of obvious > what they're for. Some places have these, some don't. It's probably more useful where a header foo.h is declaring functions that aren't in the obviously corresponding foo.c file, or live in multiple files. In this case I agree it's not adding much. regards, tom lane
        • Jump to comment-1
          john.naylor@enterprisedb.com2022-08-02T04:43:19+00:00
          On Mon, Aug 1, 2022 at 9:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > John Naylor <john.naylor@enterprisedb.com> writes: > > You are correct, but I wonder if it'd be better to just drop the comment > > entirely. I checked a couple other random headers with function > > declarations and they didn't have such a comment, and it's kind of obvious > > what they're for. > > Some places have these, some don't. It's probably more useful where > a header foo.h is declaring functions that aren't in the obviously > corresponding foo.c file, or live in multiple files. In this case > I agree it's not adding much. I somehow forgot that just yesterday I working on a project that will possibly add a declaration to every catalog header for tuple deforming. In that case, we will want to keep existing comments and possibly add more. In the meantime, I'll go just apply the correction. -- John Naylor EDB: http://www.enterprisedb.com
        • Jump to comment-1
          japinli@hotmail.com2022-08-01T14:42:22+00:00
          On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > John Naylor <john.naylor@enterprisedb.com> writes: >> You are correct, but I wonder if it'd be better to just drop the comment >> entirely. I checked a couple other random headers with function >> declarations and they didn't have such a comment, and it's kind of obvious >> what they're for. > > Some places have these, some don't. It's probably more useful where > a header foo.h is declaring functions that aren't in the obviously > corresponding foo.c file, or live in multiple files. In this case > I agree it's not adding much. > Attached patch to remove this comment. Please take a look. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
          • Jump to comment-1
            guofenglinux@gmail.com2022-08-02T03:06:01+00:00
            On Mon, Aug 1, 2022 at 10:42 PM Japin Li <japinli@hotmail.com> wrote: > > On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > John Naylor <john.naylor@enterprisedb.com> writes: > >> You are correct, but I wonder if it'd be better to just drop the comment > >> entirely. I checked a couple other random headers with function > >> declarations and they didn't have such a comment, and it's kind of > obvious > >> what they're for. > > > > Some places have these, some don't. It's probably more useful where > > a header foo.h is declaring functions that aren't in the obviously > > corresponding foo.c file, or live in multiple files. In this case > > I agree it's not adding much. > > > > Attached patch to remove this comment. Please take a look. I'm not sure that we should remove such comments. And a rough search shows that there are much more places with this kind of comments, such as below: nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal procsignal sinvaladt logtape rangetypes Thanks Richard
            • Jump to comment-1
              japinli@hotmail.com2022-08-02T04:34:29+00:00
              On Tue, 02 Aug 2022 at 11:06, Richard Guo <guofenglinux@gmail.com> wrote: > On Mon, Aug 1, 2022 at 10:42 PM Japin Li <japinli@hotmail.com> wrote: > >> >> On Mon, 01 Aug 2022 at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > John Naylor <john.naylor@enterprisedb.com> writes: >> >> You are correct, but I wonder if it'd be better to just drop the comment >> >> entirely. I checked a couple other random headers with function >> >> declarations and they didn't have such a comment, and it's kind of >> obvious >> >> what they're for. >> > >> > Some places have these, some don't. It's probably more useful where >> > a header foo.h is declaring functions that aren't in the obviously >> > corresponding foo.c file, or live in multiple files. In this case >> > I agree it's not adding much. >> > >> >> Attached patch to remove this comment. Please take a look. > > > I'm not sure that we should remove such comments. And a rough search > shows that there are much more places with this kind of comments, such > as below: > > nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal > procsignal sinvaladt logtape rangetypes > Thanks for your review! Here, I think we are only talking about catalog headers. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
            • Jump to comment-1
              john.naylor@enterprisedb.com2022-08-02T04:13:25+00:00
              On Tue, Aug 2, 2022 at 10:06 AM Richard Guo <guofenglinux@gmail.com> wrote: > > I'm not sure that we should remove such comments. And a rough search > shows that there are much more places with this kind of comments, such > as below: > > nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal > procsignal sinvaladt logtape rangetypes I was talking only about catalog/pg_*.c functions, as in Japin Li's latest patch. You didn't mention whether your examples fall in the category Tom mentioned upthread, so I'm not sure what your angle is. -- John Naylor EDB: http://www.enterprisedb.com
              • Jump to comment-1
                guofenglinux@gmail.com2022-08-02T07:37:58+00:00
                On Tue, Aug 2, 2022 at 12:13 PM John Naylor <john.naylor@enterprisedb.com> wrote: > > On Tue, Aug 2, 2022 at 10:06 AM Richard Guo <guofenglinux@gmail.com> > wrote: > > > > I'm not sure that we should remove such comments. And a rough search > > shows that there are much more places with this kind of comments, such > > as below: > > > > nbtxlog transam readfuncs walreceiver buffile bufmgr fd latch pmsignal > > procsignal sinvaladt logtape rangetypes > > I was talking only about catalog/pg_*.c functions, as in Japin Li's latest > patch. You didn't mention whether your examples fall in the category Tom > mentioned upthread, so I'm not sure what your angle is. > Sorry I forgot to mention that. The examples listed upthread all contain such comment in foo.h saying 'prototypes for functions in foo.c'. For instance, in buffile.h, there is comment saying /* * prototypes for functions in buffile.c */ So if we remove such comments, should we also do so for those cases? Thanks Richard
                • Jump to comment-1
                  daniel@yesql.se2022-08-02T07:45:36+00:00
                  > On 2 Aug 2022, at 09:37, Richard Guo <guofenglinux@gmail.com> wrote: > The examples listed upthread all contain such comment in foo.h saying > 'prototypes for functions in foo.c'. For instance, in buffile.h, there is > comment saying > /* > * prototypes for functions in buffile.c > */ > > So if we remove such comments, should we also do so for those cases? Comments which state the obvious are seldom helpful, I would prefer to remove such comments and only explicitly call out the .c file in a comment when it's a different basename from the header. -- Daniel Gustafsson https://vmware.com/
                  • Jump to comment-1
                    japinli@hotmail.com2022-08-02T07:58:40+00:00
                    On Tue, 02 Aug 2022 at 15:45, Daniel Gustafsson <daniel@yesql.se> wrote: >> On 2 Aug 2022, at 09:37, Richard Guo <guofenglinux@gmail.com> wrote: > >> The examples listed upthread all contain such comment in foo.h saying >> 'prototypes for functions in foo.c'. For instance, in buffile.h, there is >> comment saying > >> /* >> * prototypes for functions in buffile.c >> */ >> >> So if we remove such comments, should we also do so for those cases? > > Comments which state the obvious are seldom helpful, I would prefer to remove > such comments and only explicitly call out the .c file in a comment when it's a > different basename from the header. +1 -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
    • Jump to comment-1
      guofenglinux@gmail.com2022-08-01T09:28:45+00:00
      On Mon, Aug 1, 2022 at 5:18 PM Japin Li <japinli@hotmail.com> wrote: > I think there is a typo in pg_db_role_setting.h, should we fix it? Definitely this is wrong. +1 for the fix. Thanks Richard